Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Integration with ROCm 4.5.2 #179

Merged
merged 7 commits into from
May 27, 2022
Merged

Integration with ROCm 4.5.2 #179

merged 7 commits into from
May 27, 2022

Conversation

jrbyrnes
Copy link
Contributor

@jrbyrnes jrbyrnes commented May 21, 2022 •
edited
Loading

This is one of several huge PRs that we are wanting to land soon.

This one in particular contains the changes required (to OptSched) to facilitate running OptSched within the context of ROCm-4.5.2.

It is a bit unrealistic to produce a clean git history for a PR with this much work, so I'm just merging the diff.

This branch works with LLVM13, but is not backwardsly compatible with prior LLVMs -- I'm not sure exactly at which point it will break. Moreover, it hasn't been tested against the x86 backend -- supporting x86 has become more of a "nice-to-have" rather than requirement.

The significant changes include:

  1. Occupancy limiting feature -- set the "Optimal" occupancy to less than 10 for certain kernels (as defined in the .ini file).
  2. Properly map LLVM registers to OptSched registers -- there has been a change in AMDGPU backend to represent registers as comprised of 16 bit lanes, while our scheduler expects registers to be comprised of 32 bit lanes. There's some logic to map the LLVM regs back to 32 bit lanes when converting to OptSched registers.
  3. General changes to LLVM / AMDGPU API -- e.g. llvm::make_unique -> std::make_unique
  4. Some stuff for building -- there will be more work on CMakeLists in subsequent PRs.

If there are any nitpicks or if anything looks weird, let me know. I don't expect anyone to do a thorough review on this.

Thanks

Quincunx271
Quincunx271 requested changes May 21, 2022
View reviewed changes
Copy link
Member

@Quincunx271 Quincunx271 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A question: Why are we deleting example/llvm7-CPU2006-cfg?

CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
lib/Wrapper/OptimizingScheduler.cpp Outdated Show resolved Hide resolved
lib/Wrapper/OptimizingScheduler.cpp Outdated Show resolved Hide resolved
lib/Wrapper/OptimizingScheduler.cpp Outdated Show resolved Hide resolved
lib/Wrapper/OptimizingScheduler.h Outdated Show resolved Hide resolved
lib/Scheduler/sched_basic_data.cpp Outdated Show resolved Hide resolved
@jrbyrnes
Copy link
Contributor Author

@Quincunx271 Thanks for you comments Justin.

I suppose I can write back the CPU2007 config files -- the way I see it, enabling x86 will be a separate project at this point. Carrying these config files along seems could result in some confusion for newer team members. I thought it best to keep them preserved via git history.

@Quincunx271
Copy link
Member

@Quincunx271 Thanks for you comments Justin.

I suppose I can write back the CPU2007 config files -- the way I see it, enabling x86 will be a separate project at this point. Carrying these config files along seems could result in some confusion for newer team members. I thought it best to keep them preserved via git history.

To clarify, I'm not saying that we need the CPU2007 config files. I used those as the basis for my CPU2017 config files. But I'm mostly wondering why we want to remove it. It seemed useful to me, but if it's not at all useful, then sure, deleting it is fine.

I haven't had a chance to respond to some of the inline comments yet.

@jrbyrnes
Copy link
Contributor Author

To clarify, I'm not saying that we need the CPU2007 config files. I used those as the basis for my CPU2017 config files. But I'm mostly wondering why we want to remove it. It seemed useful to me, but if it's not at all useful, then sure, deleting it is fine.

Sounds like they are potentially useful -- added them back.

JoshHuttonCode
JoshHuttonCode requested changes May 24, 2022
View reviewed changes
Copy link

@JoshHuttonCode JoshHuttonCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had two issues that would not let me compile.

include/opt-sched/Scheduler/machine_model.h Outdated Show resolved Hide resolved
lib/Scheduler/list_sched.cpp Outdated Show resolved Hide resolved
@jrbyrnes jrbyrnes force-pushed the NewAMDLLVMENv branch 2 times, most recently from bb1effd to 0c26d76 Compare May 24, 2022 19:14
Quincunx271
Quincunx271 approved these changes May 25, 2022
View reviewed changes
Copy link
Member

@Quincunx271 Quincunx271 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to say thanks for the thorough updates. My review was pretty heavy and I made a lot of suggestions. Unfortunately, I don't have the time to try to understand the semantics of the change and do a semantic review, but from the resolved comments and other discussion, I think what you've done here is great.

I'm going to approve this for now. Feel free to request a re-review from me again, and if you want me to, I can look through the code again rather than just scan and go through the review comments.

lib/Wrapper/AMDGPU/OptSchedDDGWrapperGCN.h Outdated Show resolved Hide resolved
@jrbyrnes
Copy link
Contributor Author

Just want to say thanks for the thorough updates. My review was pretty heavy and I made a lot of suggestions. Unfortunately, I don't have the time to try to understand the semantics of the change and do a semantic review, but from the resolved comments and other discussion, I think what you've done here is great.

No problem, also thank you for pointing out what you did. Personally, I have some trouble finding everything for large PRs like this - so such reviews are extremely helpful. I do think it is unrealistic to expect a semantic review, so that is no problem.

@jrbyrnes jrbyrnes mentioned this pull request May 26, 2022
Open
@JoshHuttonCode JoshHuttonCode self-requested a review May 26, 2022 22:55
JoshHuttonCode
JoshHuttonCode approved these changes May 26, 2022
View reviewed changes
Copy link

@JoshHuttonCode JoshHuttonCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jrbyrnes
Copy link
Contributor Author

Do: Reformat

@jrbyrnes
Copy link
Contributor Author

Do: Run Checks

@github-actions
Copy link

github-actions bot commented May 27, 2022 •
edited
Loading

Rerunning checks: https://github.com/CSUS-LLVM/OptSched/actions/runs/2398376368

Check Status
C++ Build ❌ Failed
Clang-Format ❌ Failed
Clang-Tidy ❌ Failed
Trailing Whitespace ❌ Failed
west-const ✔ Passed

@jrbyrnes jrbyrnes merged commit 7c6ce6b into CSUS-LLVM:master May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoshHuttonCode JoshHuttonCode JoshHuttonCode approved these changes

@Quincunx271 Quincunx271 Quincunx271 approved these changes

@vangthao95 vangthao95 Awaiting requested review from vangthao95

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jrbyrnes @Quincunx271 @JoshHuttonCode